Skip to content
This repository was archived by the owner on Jul 24, 2026. It is now read-only.

convoy run declares, launches, and attaches (supersedes #92) - #98

Draft
schickling-assistant wants to merge 2 commits into
mainfrom
schickling-assistant/2026-07-21-run-declares
Draft

convoy run declares, launches, and attaches (supersedes #92)#98
schickling-assistant wants to merge 2 commits into
mainfrom
schickling-assistant/2026-07-21-run-declares

Conversation

@schickling-assistant

@schickling-assistant schickling-assistant commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

This supersedes the design that shipped in #92

If you read #92, read this section first — otherwise this looks like a regression.

#92 shipped convoy run as an ad-hoc session, and said so in the help text: "launch an AD-HOC session — NOT declared, NOT reconciled, NOT respawned, no durable context (declare it with add if it should survive)." This PR replaces that with the intended semantics: convoy run is exactly convoy add, plus a launch and an attach.

The verb set becomes coherent:

verb does
add declare (no launch)
render materialize the overlay (no launch, no bus)
up reconcile — launch what is declared
run declare, launch, and attach

Why #92's reasoning does not survive

#92 argued the declared model was "not implementable honestly", on three grounds.

1. "supervisor is required for every non-root agent, so an ephemeral declaration would fabricate one and corrupt the supervision tree."Refuted. No such field requirement exists. parseAgentFile requires only identity and role; supervisor is optional throughout. This was factually wrong.

2. "The catalog syncs union/no-delete, so a per-run entry is permanent fleet-wide garbage." — Stops being an objection under this model. The entry is not garbage: it is a declared agent that happens to have been started interactively. Permanence is the intent, not a leak. Retirement is what retired = true is for.

3. "strategy accepts only permanent." — Consistent rather than obstructive, since a run-created agent is a real declared agent. --permanent is now accepted by run (#92 rejected it with rc=2).

The deeper reason

An undeclared session has no stable identity → no durable context/ directory → it cannot externalize state or converge after a restart. That is a permanent second class of session, one that could never be promoted. Making run declare means every agent gets the same guarantees, and the ad-hoc/declared distinction disappears rather than being managed forever.

The coherence property that makes this safe

convoy up's reconcile keys liveness on the bus id <host>.<identity> (reconcile.ts agentBusId), and run launches through the very same nativeLaunch that up's reconcile uses, pinning the same identity-derived session id (writePtyTomlsessionId(spec)). So a session started by run is adopted by a concurrently-running convoy up — never double-spawned. Declaring and launching in one breath is indistinguishable, to every other verb, from add followed by up.

This was the load-bearing unknown and it was verified in the source before anything was built, not assumed.

Decisions you should push back on if you disagree

Identity: required, never generated

run now requires --identity, exactly as add does. #92 generated one (run-b3ur0v) when it was omitted.

A generated identity reintroduces precisely the hashed-name problem that makes durable context unreachable: nobody re-derives a random name on the next cold boot, so the agent can never find its own context/ again. Deriving from the role (worker-1) is worse — a counter re-derives within a parent's lifetime, so after a restart worker-2 names a different agent and reads a stranger's memory (this is #88 item 6, and #92 itself cited it as the reason its discriminator was random).

The error message explains the why, since #92 accepted the omission happily:

--identity is required. convoy run DECLARES the agent, and a declared agent needs a name you can re-derive — that is what lets its context/ survive a restart.

Attach / detach

Detaching leaves the agent running. This is free from pty's model (a session outlives its client) and it is the correct semantic here: the agent is declared, so convoy up also reconciles and respawns it. The printed notice states this rather than leaving it to be discovered — it is the exact inverse of #92's adHocNotice, which existed to disclaim these guarantees.

--no-attach covers non-interactive callers (declare + launch, return).

Already-exists: a decision table, NOT add's refuse-without---force

This is a deliberate divergence from "exactly like add", and the one place a reviewer holding me to that phrase should look:

declared live --force action
no declare + launch + attach
yes no no resume from the existing declaration
yes yes no attach to the running session
yes no yes redeclare with these flags, then launch
yes yes yes refuseconvoy reload <id>

The resume row is why. add refuses a re-declare without --force; inheriting that flatly would make re-running an identity you already declared an error — breaking the exact property this redesign exists to deliver (durable context survives a restart, so resuming is the everyday path, not a collision).

The last row refuses rather than growing a second kill-and-respawn: overwriting a live agent's declaration and relaunching would spawn a second session on the same pinned session id. convoy reload already does that job properly.

On resume/attach the existing declaration is authoritative — a stray --model must not mutate a synced, fleet-visible agent file as a side effect of attaching. Ignored flags are named out loud rather than silently dropped (staleFlagsNote), since silent ignoring is a real footgun.

Two bugs this surfaced

Liveness was keyed on the wrong host. buildDeclaration always populates host (--host ?? this machine), so keying liveness on the args-built agent file computed a this-machine bus id for an agent declared host = otherbox — whose catalog file is present locally via fabric sync. convoy run --identity <that agent> would have found nothing live and launched a duplicate alongside the real session, falsifying the coherence property above. run now reads the existing declaration before computing the bus id and keys off it, exactly as reconcile does.

The choice is a named, exported function (livenessAgentFile) rather than an inline ??, because it is the single point where the two verbs agree or diverge. Reverting it to return the args-built file turns two tests red.

One ordering bug

: buildDeclaration refused "no workspace" before knowing the agent was already declared — which would have forced callers to re-specify --dir on every resume, repeating what the catalog already knows. The check is now skipped when run is resuming an existing declaration; add keeps the hard requirement, since it only ever authors a declaration and has nothing to fall back on.

What was removed

The ad-hoc machinery is deleted, not left dormant — two models in the codebase is the thing being fixed:

  • generateAdHocIdentity, adHocNotice, isAdHocIdentity, AD_HOC_PREFIX
  • validateRunIdentity — note its premise inverted: it refused an identity colliding with a declared agent, and that collision is now precisely the resume case. Not dead code; wrong code. add's identityErrors covers grammar and socket-budget validation.
  • --prefix and --config-dir dropped from run: the declaration carries the host prefix (--host) and the harness config dir (via env), and a per-invocation override of either would desync the session from the agent file describing it.

add and run now share buildDeclaration, so they cannot drift — enforced by a test asserting run's flag set is exactly add's plus --no-attach.

Help text updated in both cli.ts and command-table.ts; the old wording actively advertised the superseded semantics.

Tests

src/run.test.ts was rewritten rather than extended — it was entirely ad-hoc machinery. 39 tests: the decision table, the messages, command-table parity with add, and end-to-end through the real CLI against throwaway networks.

Proven red without the change: applying this test file to unmodified main gives 35 failed / 4 passed. The 4 that pass are unchanged-behavior guards (unknown flag, unknown role, identity grammar validation, table presence). Every ACCEPTANCE: test and every behavior-change test fails on baseline.

Full suite: 548 passed, with 4 pre-existing environmental failures identical to origin/main (bash-3.2 heredoc parsing in doctor/hooks, git worktree in launch/network-config). No regressions.

Reviewer notes — what I did not verify

  • No live launch was exercised. Every end-to-end test runs --dry-run or --no-attach against a throwaway network. The nativeLaunchpty attach handoff is verified by construction (I confirmed writePtyToml pins the harness session to sessionId(spec) and spawnFromPtyFile spawns under exactly that name, so the attach ref is right) but never by spawning a real harness. Production was not disturbed.
  • execInteractive is new (spawn with stdio: "inherit") because exec.ts's run buffers through execFile, which would break raw mode and the detach key. Its behavior under a non-TTY stdin is untested.
  • An earlier version of the host-keying test was worthless and I nearly shipped it: it asserted the printed session ref, which was derived from the existing declaration and therefore correct either way. I only caught it by reverting the fix and watching the test still pass. The replacement tests the decision directly. Worth knowing that I check tests by breaking the code, not by watching them go green.
  • The megarepo worktree cut still does not happen at run timenativeLaunch mkdirs the workspace and writes the overlay, matching what up does today (launch.ts calls megarepo worktree-cutting a follow-up). So run inherits that gap rather than fixing it; a run against a megarepo-configured network gets a plain directory where a worktree is intended, exactly as up does.

schickling-assistant and others added 2 commits July 21, 2026 14:09
`convoy run` shipped in #92 as an AD-HOC session: deliberately not declared,
not reconciled, not respawned, no durable context. This replaces that with the
intended semantics — `run` is exactly `convoy add`, plus a launch and an attach.

The verb set becomes coherent:

  add     declare                              (no launch)
  render  materialize the overlay              (no launch, no bus)
  up      reconcile — launch what is declared  (no attach)
  run     declare, launch, and attach

The deeper reason: an undeclared session has no stable identity, therefore no
durable context/ directory, therefore it cannot externalize state or converge
after a restart. That is a permanent second class of session. Making `run`
declare means every agent gets the same guarantees and the ad-hoc/declared
distinction disappears rather than being managed forever.

`add` and `run` now share `buildDeclaration`, so the two verbs cannot drift.

Identity is REQUIRED, as for `add`. #92 generated one (`run-b3ur0v`) when it
was omitted; that is precisely the hashed-name problem that makes durable
context unreachable — nobody re-derives a random name on the next cold boot.

Attach/detach: detaching leaves the agent running (a pty session outlives its
client), and because it is declared, `convoy up` reconciles and respawns it.
`--no-attach` covers non-interactive callers.

Already-exists is a decision table, NOT `add`'s flat refuse-without-force:
  not declared                → declare + launch + attach
  declared, not live          → RESUME from the existing declaration (the
                                point of declaring; refusing would break it)
  declared, live              → attach to the running session, never restart
  declared, not live, --force → re-declare with these flags, then launch
  declared, live, --force     → refuse; `convoy reload` is the kill+respawn verb

Removed the ad-hoc machinery rather than leaving two models: generateAdHocIdentity,
adHocNotice, isAdHocIdentity, AD_HOC_PREFIX, and validateRunIdentity (whose
premise inverted — collision-with-declared is now the resume case).

Help text in cli.ts and the command table no longer advertise the old semantics.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ePNMmLYa7qVT3h7bRCWUJ
agent-session-id: 0abcedc7-6b71-4046-9e7c-f645268c0b15
agent-tool: Claude Code
agent-tool-version: 2.1.215
agent-model: claude-opus-4-8
agent-runtime-profile: /nix/store/acr8a3l2v366jgmwiq8xdrhgz1py0db5-coding-agent-runtime-profile/share/coding-agents/profile.json
agent-skills-manifest: /nix/store/sj1v5j91h8v8d1w9lca4040302lwrd6v-agent-skills-corpus/share/agent-skills/manifest.json
tooling-profile: dotfiles@unknown-dirty
`buildDeclaration` always populates `host` (`--host` ?? this machine), so
keying liveness on the args-built agent file computed a this-machine bus id
for an agent declared `host = otherbox` — whose catalog file is present
locally via fabric sync. `convoy run --identity <that agent>` would find
nothing live and launch a DUPLICATE alongside the real session.

That falsified the property this design rests on: `run` and `up` must agree
on what "already running" means. `up` reconciles on `entry.af.host ?? thisHost`
— the declaration's host — so `run` now reads the existing declaration BEFORE
computing the bus id and keys off it.

The choice is a named, exported function (`livenessAgentFile`) rather than an
inline `??`: it is the single point where the two verbs agree or diverge, so
it deserves to be visible and directly testable. Reverting it to return the
args-built file turns two tests red.

Also, while reading the existing declaration up front:
  · reuse it for `effective` instead of re-reading the file
  · warn when a positional ROLE differs from the declared one — the role is not
    a flag, so `passedDeclarationFlags` could not see it and it was silently
    dropped on resume
  · print `session:` / `bus:` ids, so a declaration owned by another host is
    visible before anything launches

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ePNMmLYa7qVT3h7bRCWUJ
agent-session-id: 0abcedc7-6b71-4046-9e7c-f645268c0b15
agent-tool: Claude Code
agent-tool-version: 2.1.215
agent-model: claude-opus-4-8
agent-runtime-profile: /nix/store/acr8a3l2v366jgmwiq8xdrhgz1py0db5-coding-agent-runtime-profile/share/coding-agents/profile.json
agent-skills-manifest: /nix/store/sj1v5j91h8v8d1w9lca4040302lwrd6v-agent-skills-corpus/share/agent-skills/manifest.json
tooling-profile: dotfiles@unknown-dirty
@schickling-assistant
schickling-assistant force-pushed the schickling-assistant/2026-07-21-run-declares branch from dab96f0 to 5bd857a Compare July 21, 2026 12:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant